Implement reading of simple key-value Logstash JSON Marker attributes #12513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
test native
oldium
force-pushed
the
feature/logstash-markers
branch
2 times, most recently
from
087cdd9
to
6f0d31f
Compare
|
Ah, Muzzle, I was afraid of that. I have no idea on what is wrong. |
About CI failing, you can refer to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/CONTRIBUTING.md#troubleshooting-pr-build-failures |
oldium
force-pushed
the
feature/logstash-markers
branch
from
6f0d31f
to
4129983
Compare
|
Thanks, hopefully fixed Muzzle. |
|
Marking as draft:
I will finish this later next week. |
oldium
changed the title
click the "Ready for review" button when you are ready for it to be reviewed, thanks! |
oldium
force-pushed
the
feature/logstash-markers
branch
from
4129983
to
fa99c59
Compare
oldium
changed the title
oldium
force-pushed
the
feature/logstash-markers
branch
from
fa99c59
to
c4b7dd5
Compare
Supported are MapEntriesAppendingMarker and SingleFieldAppendingMarker (i.e. ObjectAppendingMarker and RawJsonAppendingMarker) only. The attribute value is sent either as boolean, long, double or String or typed-array with respective values. The generic types (Object[], Collection) is converted to String array with values converted with String.valueOf() method. Typically the Logstash markers are added to logs via Markers.append(), Markers.appendEntries(), Markers.appendArray() and Markers.appendRaw() methods. Signed-off-by: Oldřich Jedlička <[email protected]>
oldium
force-pushed
the
feature/logstash-markers
branch
from
c4b7dd5
to
fea5c4d
Compare
|
Updated, rebased, ready for review. It looks like the build errors are unrelated (Lettuce). |
laurit
reviewed
Nov 6, 2024
Comment on lines
+606
to
+622
| try { | ||
| Class.forName("net.logstash.logback.marker.LogstashMarker"); | ||
| } catch (ClassNotFoundException e) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| Class.forName("net.logstash.logback.marker.SingleFieldAppendingMarker"); | ||
| } catch (ClassNotFoundException e) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| Class.forName("net.logstash.logback.marker.MapEntriesAppendingMarker"); | ||
| } catch (ClassNotFoundException e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
you could have all Class.forName calls inside the same try-catch block
laurit
reviewed
Nov 6, 2024
| @@ -60,6 +70,8 @@ public final class LoggingEventMapper { | |||
| private static final AttributeKey<List<String>> LOG_BODY_PARAMETERS = | |||
| AttributeKey.stringArrayKey("log.body.parameters"); | |||
|
|
|||
| private static final Cache<Class<?>, Field> valueField = Cache.bounded(20); | |||
There was a problem hiding this comment.
Perhaps ClassValue would serve better for caching the fields?
laurit
reviewed
Nov 6, 2024
| } | ||
| } | ||
|
|
||
| private static void captureKeyValueAttribute( |
There was a problem hiding this comment.
since this method isn't only used for key value attributes any more maybe it should be renamed
laurit
reviewed
Nov 6, 2024
| } | ||
| } | ||
|
|
||
| private static <T> void captureKeyArrayValueAttribute( |
There was a problem hiding this comment.
I think it would be easier to write the converted values to a list.
private static <T> void captureKeyArrayValueAttribute(
AttributesBuilder attributes,
AttributeKey<List<T>> key,
Object array,
Function<Object, T> extractor) {
List<T> list = new ArrayList<>();
int length = java.lang.reflect.Array.getLength(array);
for (int i = 0; i < length; i++) {
Object value = (T) java.lang.reflect.Array.get(array, i);
if (value != null) {
list.add(extractor.apply(value));
}
}
// empty lists are not serialized
if (!list.isEmpty()) {
attributes.put(key, list);
}
}
laurit
reviewed
Nov 6, 2024
| } else if (value instanceof Double || value instanceof Float) { | ||
| attributes.put(keyStr, ((Number) value).doubleValue()); | ||
| } else if (value.getClass().isArray()) { | ||
| if (value instanceof boolean[] || value instanceof Boolean[]) { |
There was a problem hiding this comment.
I think we need a unit test for this method to ensure that all the array variations are tested.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supported are
MapEntriesAppendingMarkerandSingleFieldAppendingMarker(i.e.ObjectAppendingMarkerandRawJsonAppendingMarker) only. The attribute value is always a string retrieved by a call totoString()method.Typically the Logstash markers are added to logs via
Markers.append()andMarkers.appendEntries()methods.Closes #12573